{ACS} az openshift show/list: Refactoring to reduce the probability of AttributeError #13358
Conversation
There was a problem hiding this comment.
It doesn't seem like we ever had name and os_type attributes in the public API.
There was a problem hiding this comment.
But this code been here since the beginning of times too. Not sure what broke the command.
There was a problem hiding this comment.
Hi @m1kola , is this because the different API version? Are you sure we can remove these code safely?
There was a problem hiding this comment.
@arrownj I'm currently not sure why it's happening. It was working fine on Azure CLI 2.3.1 (maybe on newer versions too) and az openshift show and az openshift list are broken on 2.5.1 and dev branch.
As far as I see, we did not change anything in the API and we never had name and os_type fields in the master pool profile API on the server (I checked all the supported versions in our codebase).
There was a problem hiding this comment.
Here's the break:
Azure/azure-sdk-for-python@3f6238b#diff-66c3074fc0745e8946eba5783a5f8f32R21
The client library we were using was stale having been generated on an ancient pre-GA API and ^ brings us up to date.
|
@julienstroheker could you also please take a look? |
|
add to S169 |
jim-minter
left a comment
jim-minter
left a comment
There was a problem hiding this comment.
I'm +1 on this change. Thanks @m1kola !
There was a problem hiding this comment.
Here's the break:
Azure/azure-sdk-for-python@3f6238b#diff-66c3074fc0745e8946eba5783a5f8f32R21
The client library we were using was stale having been generated on an ancient pre-GA API and ^ brings us up to date.
There was a problem hiding this comment.
| if getattr(managed_cluster, attr, None) is None: | |
| if hasattr(managed_cluster, attr) and getattr(managed_cluster, attr) is None: |
arguably ^ is better, but it doesn't really matter.
There was a problem hiding this comment.
If remember Python correctly getattr(managed_cluster, attr) still needs to be getattr(managed_cluster, attr, None). Otherwise there will be an exception in case when there is no attribute.
There was a problem hiding this comment.
But it makes sense. I'll update.
There was a problem hiding this comment.
Ahh. I see... hasattr :) Tired brain.
It was failing with a Python Traceback
m1kola
force-pushed
the
fixes_az_openshift_show
branch
from
76b3ae4 to
94afb75
Compare
|
It looks like it is a duplicated of #13315 |
m1kola
changed the title
|
The changes in the original PR were a duplicate of #13315 which got merged recently. I rebased the PR and I updated it so we hopefully reduce the probability of Please take another look. |
|
LGTM. |
5 participants
Description
The changes in the original PR were a duplicate of #13315 which got merged recently. I rebased the PR and I updated it so we hopefully reduce the probability of
AttributeErrorin the similar cases.This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.